-
Notifications
You must be signed in to change notification settings - Fork 30
Add support for .ear files and a less naive short-circuit file checker. #17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This support allows .ear files to be seen as zip files (since they are typically), and expands the quick check for java class files to search in nested archives instead of only the top-most. Fixed (via workaround) a cleanup issue that only exhibits in development workflows. This creates cleanup phases of preshutdown and shutdown. This still needs to be managed carefully. A better solution would define strict dependency (or have better error checking in the callbacks).
| .map(_.toOption.flatMap { Option(_) }) | ||
| .takeWhile(_.isDefined) | ||
| .flatten | ||
| .filterNot(ZipCleaner.shouldFilter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation around here is all fudged up
| entryStream.exists(entry => | ||
| callback(filename, entry, zipStream) | ||
| ||(recursive && isZip(entry.getName) && findFirstEntry(s"$filename/${entry.getName}", new CloseShieldInputStream(zipStream), true)(callback)) | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Format nitpicks:
- Typically with a multi-line closure, you'd use curlies instead of parentheses, e.g.
{ entry => ... } - Space after the
||
| case _ => false | ||
| } | ||
| } else { | ||
| false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The predicate passed to findFirstEntry here is logically equivalent to the one used in checkForBinaryZip (a few lines above this function). (The slight difference is that FilenameUtils will deal with the edge case of a folder named "foo.class", but that doesn't matter since it's in a guard against directories anyway).
You could just use !entry.isDirectory && FilenameUtils.getExtension(entry.getName) == "class"
| } | ||
| } | ||
|
|
||
| def findFirstEntry(file: File, recursive: Boolean = true)(callback: (String, ZipEntry, InputStream) => Boolean): Boolean = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming nitpick; predicate instead of callback since it returns a Boolean
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence about having a default value for the recursive parameter. My gut reaction was that it should be explicit, but there's nothing exactly wrong with it being default. Up to you.
This support allows .ear files to be seen as zip files (since they are typically), and expands the quick check for java class files to search in nested archives instead of only the top-most.
Fixed (via workaround) a cleanup issue that only exhibits in development workflows. This creates cleanup phases of preshutdown and shutdown. This still needs to be managed carefully. A better solution would define strict dependency (or have better error checking in the callbacks).